From 06a0605f7b93132870a4e2ec052059250c624131 Mon Sep 17 00:00:00 2001
From: Alex Crichton <alex@alexcrichton.com>
Date: Fri, 12 Jun 2015 15:03:15 -0700
Subject: [PATCH] Don't rely on Hash for Package deduplicating

The fix in #1697 was to alter the `Hash` implementation of `Package` to take
into account the source root instead of just the package id (so packages at the
same source would have the same hash). There's a spot, however, where we
normalize some paths and not others, causing two paths which logically point the
same location end up having different hashes, so the same package ended up being
stored multiple times, later leading to an assertion that there was only one
stored.

This commit alters the behavior of read_packages to keep a hash map of ID =>
Package instead of just a hash set of Package as the deduplication/equality
needs to be based on the ID, not the literal path to the source root.

This specific bug shows up when you have an override and a normal dependency
pointing at the same location (and the override has some inter-dependencies).
Not exactly a normal use case, but it showed up in Servo's build!
---
 src/cargo/ops/cargo_read_manifest.rs  | 12 ++++----
 tests/test_cargo_compile_path_deps.rs | 41 +++++++++++++++++++++++++++
 2 files changed, 47 insertions(+), 6 deletions(-)

diff --git a/src/cargo/ops/cargo_read_manifest.rs b/src/cargo/ops/cargo_read_manifest.rs
index f9027404d..850785326 100644
--- a/src/cargo/ops/cargo_read_manifest.rs
+++ b/src/cargo/ops/cargo_read_manifest.rs
@@ -1,10 +1,10 @@
-use std::collections::HashSet;
+use std::collections::{HashMap, HashSet};
 use std::fs::{self, File};
 use std::io::prelude::*;
 use std::io;
 use std::path::{Path, PathBuf};
 
-use core::{Package,Manifest,SourceId};
+use core::{Package, Manifest, SourceId, PackageId};
 use util::{self, CargoResult, human, Config, ChainError};
 use util::important_paths::find_project_manifest_exact;
 use util::toml::{Layout, project_layout};
@@ -35,7 +35,7 @@ pub fn read_package(path: &Path, source_id: &SourceId, config: &Config)
 
 pub fn read_packages(path: &Path, source_id: &SourceId, config: &Config)
                      -> CargoResult<Vec<Package>> {
-    let mut all_packages = HashSet::new();
+    let mut all_packages = HashMap::new();
     let mut visited = HashSet::<PathBuf>::new();
 
     trace!("looking for root package: {}, source_id={}", path.display(), source_id);
@@ -69,7 +69,7 @@ pub fn read_packages(path: &Path, source_id: &SourceId, config: &Config)
     if all_packages.is_empty() {
         Err(human(format!("Could not find Cargo.toml in `{}`", path.display())))
     } else {
-        Ok(all_packages.into_iter().collect())
+        Ok(all_packages.into_iter().map(|(_, v)| v).collect())
     }
 }
 
@@ -103,7 +103,7 @@ fn has_manifest(path: &Path) -> bool {
 }
 
 fn read_nested_packages(path: &Path,
-                        all_packages: &mut HashSet<Package>,
+                        all_packages: &mut HashMap<PackageId, Package>,
                         source_id: &SourceId,
                         config: &Config,
                         visited: &mut HashSet<PathBuf>) -> CargoResult<()> {
@@ -112,7 +112,7 @@ fn read_nested_packages(path: &Path,
     let manifest = try!(find_project_manifest_exact(path, "Cargo.toml"));
 
     let (pkg, nested) = try!(read_package(&manifest, source_id, config));
-    all_packages.insert(pkg);
+    all_packages.insert(pkg.package_id().clone(), pkg);
 
     // Registry sources are not allowed to have `path=` dependencies because
     // they're all translated to actual registry dependencies.
diff --git a/tests/test_cargo_compile_path_deps.rs b/tests/test_cargo_compile_path_deps.rs
index 13586953a..87174cc42 100644
--- a/tests/test_cargo_compile_path_deps.rs
+++ b/tests/test_cargo_compile_path_deps.rs
@@ -793,3 +793,44 @@ test!(custom_target_no_rebuild {
 {compiling} b v0.5.0 ([..])
 ", compiling = COMPILING)));
 });
+
+test!(override_and_depend {
+    let p = project("foo")
+        .file("a/a1/Cargo.toml", r#"
+            [project]
+            name = "a1"
+            version = "0.5.0"
+            authors = []
+            [dependencies]
+            a2 = { path = "../a2" }
+        "#)
+        .file("a/a1/src/lib.rs", "")
+        .file("a/a2/Cargo.toml", r#"
+            [project]
+            name = "a2"
+            version = "0.5.0"
+            authors = []
+        "#)
+        .file("a/a2/src/lib.rs", "")
+        .file("b/Cargo.toml", r#"
+            [project]
+            name = "b"
+            version = "0.5.0"
+            authors = []
+            [dependencies]
+            a1 = { path = "../a/a1" }
+            a2 = { path = "../a/a2" }
+        "#)
+        .file("b/src/lib.rs", "")
+        .file("b/.cargo/config", r#"
+            paths = ["../a"]
+        "#);
+    p.build();
+    assert_that(p.cargo("build").cwd(p.root().join("b")),
+                execs().with_status(0)
+                       .with_stdout(&format!("\
+{compiling} a2 v0.5.0 ([..])
+{compiling} a1 v0.5.0 ([..])
+{compiling} b v0.5.0 ([..])
+", compiling = COMPILING)));
+});
-- 
2.30.2